Pc/feat/nav bar update #881

PatrickCleary · 2023-10-21T22:11:37Z

Motivation

Remove the tabs for sidebar navigation.
#848

Changes

Local data is broken for me for some reason, that's why all the graphs are erroring out.

Screen.Recording.2023-11-24.at.1.25.34.PM.mov

Testing Instructions

This change is pretty massive, so would appreciate thorough testing of all the pages, devices, different configurations, etc. It almost certainly has at least a couple bugs.

I will be doing some additional review at some point before I merge this.

PatrickCleary · 2023-11-24T18:22:22Z

.prettierrc

I couldn't get prettier to work until I changed this. Is this a bad idea? @devinmatte

Hmm did a new update break .js prettierrcs?

idreyn

This is a really huge improvement, I think. I have a few stray design thoughts for now, I haven't looked at the code yet.

With reference to the above image, I wonder whether we can drop the Bus and Subway headings. Red/Orange/Blue/Green/Bus as top-level items seem fine. Having a Bus heading with one subentry, Buses, is throwing me off a bit.

If we keep them, I think we should consider padding below the headings instead of to the left of the sub-items. For example:

Some menu items like global Slow Zones don't have hover/active states yet. I also think those should retain some icons.
Are icons for the per-line subpages (service etc) a no-go at this resolution? I kinda liked them but I can see how they'd be a bit busy here.
The bus line dropdown (white text on yellow) is really hard for me to read. Could we use a more traditional black text on white here? It might also look better to expand the dropdown so that it entirely fills the yellow rectangle that contains it, with no padding or rounded corners.

PatrickCleary · 2023-11-27T22:58:24Z

Thanks for checking this out. All good points. 1. Works for me 2. I thought I had added those. Might’ve forgot to push the last commit? either way, definitely agree those are needed. 3. Yea, I had the icons at first and it was a bit much. I can send a pic of what it would look like. Maybe we can incorporate them elsewhere? 4. will do

…

On Mon, Nov 27, 2023 at 11:03 Ian Reynolds ***@***.***> wrote: ***@***.**** commented on this pull request. This is a really huge improvement, I think. I have a few stray design thoughts for now, I haven't looked at the code yet. image.png (view on web) <https://github.com/transitmatters/t-performance-dash/assets/2208769/cf438eaf-8cda-49ea-ba3b-469be0f334c1> 1. With reference to the above image, I wonder whether we can drop the *Bus* and *Subway* headings. Red/Orange/Blue/Green/Bus as top-level items seem fine. Having a *Bus* heading with one subentry, Buses, is throwing me off a bit. If we keep them, I think we should consider padding *below* the headings instead of to the *left* of the sub-items. For example: image.png (view on web) <https://github.com/transitmatters/t-performance-dash/assets/2208769/647e1663-7991-4c17-aa3b-9feff709b1ea> 1. Some menu items like global Slow Zones don't have hover/active states yet. I also think those should retain some icons. 2. Are icons for the per-line subpages (service etc) a no-go at this resolution? I kinda liked them but I can see how they'd be a bit busy here. 3. The bus line dropdown (white text on yellow) is really hard for me to read. Could we use a more traditional black text on white here? It might also look better to expand the dropdown so that it entirely fills the yellow rectangle that contains it, with no padding or rounded corners. image.png (view on web) <https://github.com/transitmatters/t-performance-dash/assets/2208769/22ed102d-3d30-4c4b-aaa7-5bf3a902b578> — Reply to this email directly, view it on GitHub <#881 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALAWOZMQS26SHN3Y6INP4X3YGS2WNAVCNFSM6AAAAAA6KNN4FSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJQHAYDQMBQHE> . You are receiving this because you were assigned.Message ID: ***@***.***>

PatrickCleary · 2023-11-28T03:00:47Z

@idreyn – Just updated with commit I had forgot to merge. Should have hover/active states for top two options (which should also still have icons).

Here is a pic of icons in the sidebar:

and without:

Bus color/style adjustment

Idk if this is what you were thinking. Let me know. Also, if you want to go in and try anything, please do.

PatrickCleary · 2023-11-28T03:02:33Z

(most recent commit has no sidebar icons and updated bus style)

idreyn · 2023-12-01T01:30:25Z

The bus dropdown is 👨‍🍳👌 and I still kinda like the icons in the sidebar tbh but that's just one opinion!

My vote is to land this basically as-is and then iterate if we want to change it. I'm setting a reminder to review the code tomorrow when I am feeling less zonked.

devinmatte · 2023-12-01T01:56:41Z

So this looks great. Pushed it to beta to make it easier to test, and see it with real data

So I'll say that I'm also partial to the icons on the sections returning, but otherwise this is really clean 👌

I found a bug where I can still click the disabled green line slowzone section and open the broken page

modules/navigation/SideNavigation.tsx

modules/navigation/ExtraMenuItems.tsx

modules/navigation/SidebarTabs.tsx

PatrickCleary · 2023-12-01T02:47:32Z

@devinmatte that is actually a pre-existing bug I believe @mathcolo — I don’t mind removing the dot, maybe a little more padding on the left tho? Also if u have the code open and want to push grammar corrections directly, pls do.

…

On Thu, Nov 30, 2023 at 21:34 Preston Mueller ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In modules/navigation/SideNavigation.tsx <#881 (comment)> : > + <FontAwesomeIcon icon={faHouse} size="lg" /> + </div> + <h2>Home</h2> + </Link> + <Link + href="/system/slowzones" + className={classNames( + page === 'systemSlowzones' ? 'bg-opacity-75' : 'bg-opacity-0', + 'flex flex-row items-center gap-2 rounded-md bg-black bg-opacity-0 hover:bg-opacity-25 hover:text-white' + )} + onClick={() => (close ? close() : undefined)} + > + <div className="flex h-8 w-8 items-center justify-center rounded-full"> + <FontAwesomeIcon icon={faWarning} size="lg" /> + </div> + <h2>Slow Zones</h2> uppercase Z is a grammar regression ------------------------------ In modules/navigation/ExtraMenuItems.tsx <#881 (comment)> : > +export const ExtraMenuItems: React.FC = () => { + return ( + <div className="flex flex-col gap-1 py-2 text-sm"> + <Link + href="https://transitmatters.org/transitmatters-labs" + className="flex flex-row items-center gap-2 text-white hover:text-blue-500" + > + <div className="h-8 w-8" /> + About + </Link> + <Link + href="https://transitmatters.org/join" + className="flex flex-row items-center gap-2 text-white hover:text-blue-500" + > + <div className="h-8 w-8" /> + Join Us uppercase U is a grammar regression ------------------------------ In modules/navigation/ExtraMenuItems.tsx <#881 (comment)> : > + <div className="h-8 w-8" /> + Join Us + </Link> + <Link + href="https://forms.gle/SKYtxgKSyCrYxM1v7" + className="flex flex-row items-center gap-2 text-white hover:text-blue-500" + > + <div className="h-8 w-8" /> + Feedback + </Link> + <Link + href="https://github.com/transitmatters/t-performance-dash" + className="flex flex-row items-center gap-2 text-white hover:text-blue-500" + > + <div className="h-8 w-8" /> + Source Code downcase C ------------------------------ In modules/navigation/SidebarTabs.tsx <#881 (comment)> : > )} > - <FontAwesomeIcon - icon={tab.icon} - aria-hidden={true} - className={classNames( - selected ? 'text-white' : 'text-stone-200', - enabled ? 'group-hover:text-white' : 'text-stone-600', - 'shrink-0', - tab.sub ? 'h-4 w-4' : 'h-6 w-6' - )} - /> + <span className="text-lg">•</span> Out of curiosity, what if the • wasn't there? ------------------------------ In modules/navigation/SidebarTabs.tsx <#881 (comment)> : > )} > - <FontAwesomeIcon - icon={tab.icon} - aria-hidden={true} - className={classNames( - selected ? 'text-white' : 'text-stone-200', - enabled ? 'group-hover:text-white' : 'text-stone-600', - 'shrink-0', - tab.sub ? 'h-4 w-4' : 'h-6 w-6' - )} - /> + <span className="text-lg">•</span> image.png (view on web) <https://github.com/transitmatters/t-performance-dash/assets/193453/2bcbde57-1953-47e3-b8b0-e87dee365b91> I might prefer this. not sure though — Reply to this email directly, view it on GitHub <#881 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALAWOZIKEEMBQNDF2TMSC5LYHE63VAVCNFSM6AAAAAA6KNN4FSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJYHEYDAMJRHA> . You are receiving this because you were assigned.Message ID: ***@***.***>

PatrickCleary · 2023-12-01T17:45:04Z

Okay I think everyone's comments have been addressed. Added back icons based on slack poll results.

@devinmatte @mathcolo @idreyn

mathcolo · 2023-12-01T18:03:47Z

Could we get the most recent changes on beta just to check it out before merge?

idreyn

Latest looks very classy.

I have one final nitpick (and really just one): in certain states, it looks like the elements could use some padding — between the selected Orange Line and the not-selected Red Line, and between the Red circle-icon and the left-edge of its rounded container.

But of course good to land as-is.

idreyn · 2023-12-01T17:58:28Z

common/components/nav/MenuDropdown.tsx

+      <Link
+        href={
+          line === 'line-bus'
+            ? `/bus/trips/single?busRoute=1&date=${BUS_DEFAULTS.singleTripConfig.date}`


It would be nice to have some helper fns to generate these URLs

idreyn · 2023-12-01T18:06:31Z

modules/navigation/SidebarTabs.tsx

@@ -11,23 +11,29 @@ import type { PageMetadata } from '../../common/constants/pages';

 interface SidebarTabs {
  tabs: PageMetadata[];
-  setSidebarOpen?: React.Dispatch<React.SetStateAction<boolean>>;
+  close?: (


This never gets called with an argument so I think it could be just close: () => void. There are a few other components wrapping this one that have the same prop, and afaict that change could be made there too.

devinmatte

Looks great. I'll echo that perhaps we need a little additional margins between the lines, but, nothing major. Looks solid!

PatrickCleary · 2023-12-01T18:36:24Z

I won't have time again today to look at it, but the feedback is all reasonable to me. If we're tryna get this in soon and someone wants to push those changes, please do.

…

On Fri, Dec 1, 2023 at 1:24 PM Devin Matte ***@***.***> wrote: ***@***.**** approved this pull request. Looks great. I'll echo that perhaps we need a little additional margins between the lines, but, nothing major. Looks solid! — Reply to this email directly, view it on GitHub <#881 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALAWOZMQFADNLTIJZ2EOYK3YHIOF5AVCNFSM6AAAAAA6KNN4FSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONRQGM2TIOJXHE> . You are receiving this because you were assigned.Message ID: ***@***.***>

PatrickCleary added 2 commits October 21, 2023 18:08

Dropdown bus and subway done. Need to add bus line selection.

2a86893

lint is broken...

61b003c

github-actions bot added the frontend Change to frontend code label Oct 21, 2023

PatrickCleary added 8 commits October 29, 2023 09:15

Fix bug with duplicate dropdowns

deb071d

more padding

4a68dca

Add bus routes dropdown

32702b6

Add borders

26b47f1

Work in Progress

07cdbb2

Fixed mobile scrolling. Align icons + text

9c3ed69

Add hover + selected logic to all buttons on sidebar.

e6f81bf

Linting (it was broken previously)

d6643a4

PatrickCleary self-assigned this Nov 24, 2023

PatrickCleary and others added 2 commits November 24, 2023 13:23

unused

9299ffb

Merge branch 'main' into pc/feat/nav-bar-update

17bd2bc

PatrickCleary marked this pull request as ready for review November 24, 2023 18:30

PatrickCleary requested review from mathcolo, idreyn, skaplan-dev and devinmatte as code owners November 24, 2023 18:30

PatrickCleary commented Nov 24, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into pc/feat/nav-bar-update

205052a

idreyn reviewed Nov 27, 2023

View reviewed changes

style modifications

c5754c0

mathcolo reviewed Dec 1, 2023

View reviewed changes

modules/navigation/SideNavigation.tsx Outdated Show resolved Hide resolved

modules/navigation/ExtraMenuItems.tsx Outdated Show resolved Hide resolved

modules/navigation/ExtraMenuItems.tsx Outdated Show resolved Hide resolved

modules/navigation/SidebarTabs.tsx Outdated Show resolved Hide resolved

PatrickCleary added 4 commits December 1, 2023 12:38

The people have spoken – icons are in.

1a5b43a

tad more paddidng

9568f42

[sic] grammer

9aa0851

lint

6abd94c

PatrickCleary requested a review from mathcolo December 1, 2023 17:44

idreyn approved these changes Dec 1, 2023

View reviewed changes

devinmatte approved these changes Dec 1, 2023

View reviewed changes

small padding changes, remove old prop type.

1e7c46c

PatrickCleary merged commit cd3d7cc into main Dec 4, 2023
5 checks passed

PatrickCleary deleted the pc/feat/nav-bar-update branch December 4, 2023 00:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pc/feat/nav bar update #881

Pc/feat/nav bar update #881

PatrickCleary commented Oct 21, 2023 •

edited by devinmatte

Loading

PatrickCleary Nov 24, 2023

devinmatte Nov 24, 2023

idreyn left a comment

PatrickCleary commented Nov 27, 2023 via email

PatrickCleary commented Nov 28, 2023

PatrickCleary commented Nov 28, 2023

idreyn commented Dec 1, 2023

devinmatte commented Dec 1, 2023

PatrickCleary commented Dec 1, 2023 via email

PatrickCleary commented Dec 1, 2023

mathcolo commented Dec 1, 2023

idreyn left a comment

idreyn Dec 1, 2023

idreyn Dec 1, 2023

devinmatte left a comment

PatrickCleary commented Dec 1, 2023 via email

Pc/feat/nav bar update #881

Pc/feat/nav bar update #881

Conversation

PatrickCleary commented Oct 21, 2023 • edited by devinmatte Loading

Motivation

Changes

Testing Instructions

PatrickCleary Nov 24, 2023

Choose a reason for hiding this comment

devinmatte Nov 24, 2023

Choose a reason for hiding this comment

idreyn left a comment

Choose a reason for hiding this comment

PatrickCleary commented Nov 27, 2023 via email

PatrickCleary commented Nov 28, 2023

PatrickCleary commented Nov 28, 2023

idreyn commented Dec 1, 2023

devinmatte commented Dec 1, 2023

PatrickCleary commented Dec 1, 2023 via email

PatrickCleary commented Dec 1, 2023

mathcolo commented Dec 1, 2023

idreyn left a comment

Choose a reason for hiding this comment

idreyn Dec 1, 2023

Choose a reason for hiding this comment

idreyn Dec 1, 2023

Choose a reason for hiding this comment

devinmatte left a comment

Choose a reason for hiding this comment

PatrickCleary commented Dec 1, 2023 via email

PatrickCleary commented Oct 21, 2023 •

edited by devinmatte

Loading